KAFKA-20617: add validation for cluster-id in Formatter by gaurav-narula · Pull Request #22384 · apache/kafka

gaurav-narula · 2026-05-27T10:48:52Z

This change adds validation for Uuids in Formatter which is used by kafka-storage.sh which allows users to fail fast before they inadvertently end up using an invalid cluster-id.

This change adds validation for Uuids in Formatter which is used by `kafka-storage.sh` which allows users to fail fast before they inadvertently end up using an invalid cluster-id.

gnarula · 2026-05-28T13:51:49Z

CC: @showuon @fvaleri

showuon · 2026-05-29T09:43:31Z

+            if (clusterId.contains("=")) {
+                throw new FormatterException("The specified cluster id, " + clusterId + " contains padding and is invalid");


Our randomUuid method is possible to output the uuid contains = sign:

This will not generate a UUID equal to 0, 1, or one whose string representation starts with a dash ("-")

So is this the expected validation?

I don't think it is possible because Uuid#toString does not use padding and = sign can only be present when padding is enabled

I think this actually covers a real gap. The one in which a CID like igNUVIdeSPO5JCZYFhOh7Q== would pass the validation, but would be returned as igNUVIdeSPO5JCZYFhOh7Q by Uuid.toString(). I would just move it outside the try-catch block as it throws FormatterException directly and would bypass the catch anyway.

showuon · 2026-05-29T09:44:33Z

+            Uuid uuid = Uuid.fromString(clusterId);
+            if (Uuid.RESERVED.contains(uuid)) {
+                throw new FormatterException("The specified cluster id, " + clusterId + " is reserved");
+            }


Let's also validate the starting - sign.

I think these are cosmetic improvemenst that've been added over time but have no bearing on correctness. Failing validation on them would hinder migration from older clusters where - was allowed.

The motivation for avoiding - in the beginning was to avoid shell escaping issues when passing cluster id in CLI tools. https://issues.apache.org/jira/browse/KAFKA-13741

Lately, https://issues.apache.org/jira/browse/KAFKA-20072 avoided - altogether to allow easier copy-pasting.

Good point. Let's add a comment about this for future reference.

fvaleri

@gaurav-narula good catch. Left some comments.

fvaleri · 2026-05-29T10:36:40Z

+
+    @ParameterizedTest
+    @ValueSource(strings = {"unrvTtQISjar0JUWGU/8Pg", "igNUVIdeSPO5JCZYFhOh7Q==", "AAAAAAAAAAAAAAAAAAAAAA", "AAAAAAAAAAAAAAAAAAAAAQ"})
+    public void testFormatWithInvalidClusterId(String clusterId) throws Exception {


There's no corresponding positive test showing that a valid CID passes validation. This would guard against the validation being too aggressive.

fvaleri · 2026-05-29T10:52:16Z

        }
+        try {
+            if (clusterId.contains("=")) {
+                throw new FormatterException("The specified cluster id, " + clusterId + " contains padding and is invalid");


Suggested change

throw new FormatterException("The specified cluster id, " + clusterId + " contains padding and is invalid");

throw new FormatterException("The specified cluster id, " + clusterId + " is invalid: contains padding");

fvaleri · 2026-05-29T10:55:48Z

+            assertEquals(expectedPrefix,
+                    assertThrows(FormatterException.class,
+                            formatter1.formatter::run).
+                            getMessage().substring(0, expectedPrefix.length()));


Wouldn't be easier to rewrite like:

assertTrue(message.startsWith(expectedPrefix))

I'm indifferent here - I followed the convention used in another test in the same class

fvaleri · 2026-05-29T12:05:59Z

+            if (clusterId.contains("=")) {
+                throw new FormatterException("The specified cluster id, " + clusterId + " contains padding and is invalid");


I think this actually covers a real gap. The one in which a CID like igNUVIdeSPO5JCZYFhOh7Q== would pass the validation, but would be returned as igNUVIdeSPO5JCZYFhOh7Q by Uuid.toString(). I would just move it outside the try-catch block as it throws FormatterException directly and would bypass the catch anyway.

fvaleri · 2026-05-29T12:10:01Z

+            Uuid uuid = Uuid.fromString(clusterId);
+            if (Uuid.RESERVED.contains(uuid)) {
+                throw new FormatterException("The specified cluster id, " + clusterId + " is reserved");
+            }


Good point. Let's add a comment about this for future reference.

fvaleri · 2026-05-29T13:28:44Z

+    /**
+     * Validates the correctness of the given cluster id. A valid cluster id is a base64, urlencoded, no padding
+     * representation of a {@link Uuid}. These checks do not validate the absence of <code>-</code> character as
+     * {@link Uuid#randomUuid()} avoids them only for convenience reasons.


I would explicitly mention support for already generated CIDs. Basically it's for historical reasons.

fvaleri

LGTM. Thanks.

KAFKA-20617: add validation for cluster-id in Formatter

ab0caec

This change adds validation for Uuids in Formatter which is used by `kafka-storage.sh` which allows users to fail fast before they inadvertently end up using an invalid cluster-id.

github-actions Bot added triage PRs from the community kraft small Small PRs labels May 27, 2026

showuon reviewed May 29, 2026

View reviewed changes

fvaleri reviewed May 29, 2026

View reviewed changes

Address review comments

2a70079

gaurav-narula force-pushed the KAFKA-20617 branch from cd317eb to 2a70079 Compare May 29, 2026 13:01

fvaleri reviewed May 29, 2026

View reviewed changes

Stress that validating - would break compatibility with old cids

55d3e00

fvaleri approved these changes May 29, 2026

View reviewed changes

github-actions Bot removed the triage PRs from the community label May 30, 2026

		if (clusterId.contains("=")) {
		throw new FormatterException("The specified cluster id, " + clusterId + " contains padding and is invalid");

Conversation

gaurav-narula commented May 27, 2026

Uh oh!

gnarula commented May 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaurav-narula May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fvaleri May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fvaleri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fvaleri May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fvaleri left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gaurav-narula May 29, 2026 •

edited

Loading

fvaleri May 29, 2026 •

edited

Loading

fvaleri May 29, 2026 •

edited

Loading